Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 2, 2025

What are you trying to accomplish?

Combines the Azure mirror configuration RUN (lines 17-59) and apt-get installation RUN (lines 61-109) into a single layer. This is the base image, so reducing layers will mean millions fewer calls from the Docker image registry.

Fixes #12341

Anything you want to highlight for special attention from reviewers?

  • Switched from RUN set -e; ... && ... chaining to RUN <<EOT heredoc syntax to cleanly embed the nested <<EOF for the sources file
  • The Azure mirror config (/etc/apt/sources.list.d/ubuntu.sources) persists in the final layer—child images still inherit it for their own apt-get calls
  • No functional changes to what gets installed or configured

How will you know you've accomplished your goal?

  • docker buildx build --check -f Dockerfile.updater-core . passes syntax validation
  • CI builds succeed
  • Layer count reduced by 1

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.
Original prompt

This section details on the original issue you should resolve

<issue_title>Merge the two apt RUN instructions in Dockerfile.updater-core</issue_title>
<issue_description>These two RUN commands can be combined

May seem trivial, but this is the base image so it will impact every job. Combining these will mean millions of fewer calls from the Docker image registry.

Note: the one thing I'm not sure of is whether this apt mirror that gets configured is intended to live beyond the immediate apt get call... for example, the child docker images that inherit from this may want it in order to ensure that any apt-get calls they make are also fast. If today they inherit this file, but combining would result in them not inheriting, then don't do any work. However, given that we explicitly rm apt config, I do wonder if this is getting all cleaned up anyway at the end of the layer?... </issue_description>

Comments on the Issue (you are @copilot in this section)

  • Fixes github/dependabot-updates#11564

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Merge the two apt RUN instructions in Dockerfile.updater-core Merge the two apt RUN instructions in Dockerfile.updater-core Dec 2, 2025
Copilot AI requested a review from jeffwidman December 2, 2025 01:32
Copilot finished work on behalf of jeffwidman December 2, 2025 01:32
apt purge software-properties-common apt-transport-https -y
apt-get autoremove -y
rm -rf /var/lib/apt/lists/*
EOT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot what does this EOT syntax do? Are these commands still executed as part of the RUN command or are they copied to a file instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RUN <<EOT ... EOT syntax is the Dockerfile heredoc feature introduced in BuildKit (Docker 1.20+). All commands between <<EOT and EOT are executed as part of the RUN instruction—nothing is copied to a file.

This syntax allows writing multi-line shell scripts more naturally without && \ line continuations. It also enables embedding nested heredocs (like the <<EOF ... EOF for creating the apt sources file) which wouldn't work with the traditional RUN syntax.

See the Docker documentation: https://docs.docker.com/build/building/best-practices/#heredocs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa, cool!

@jeffwidman
Copy link
Member

@yeikel what's your take on this?

We have an issue where millions of Dependabot jobs are resulting in each docker pull hitting the registry for each docker image layer. So we wanted to reduce layers where it made sense (ie, don't be myopically fixated on a single layer) and here I wondered if we could accomplish both RUN commands as one... so I asked Copilot to put together a PR to do that...

What do you think of this PR?

Copilot AI requested a review from jeffwidman December 2, 2025 01:41
Copilot finished work on behalf of jeffwidman December 2, 2025 01:41
mercurial \
# needed to sign commits
gnupg2 \
# for git repo access with ssh key credentials
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are important context, can we ask copilot to bring them back? @jeffwidman

That, or bring them back manually. Whatever works best

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot can you please leave the code comments inline and not delete them?

Copy link
Member

@jeffwidman jeffwidman Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yeikel good spot. I was only looking at the actual dockerfile RUN structure change and completely missed that!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like I'll have to manually fixup this commit... that's okay, at least we're a lot closer. Let's see if CI is happy

@yeikel
Copy link
Contributor

yeikel commented Dec 2, 2025

@yeikel what's your take on this?

We have an issue where millions of Dependabot jobs are resulting in each docker pull hitting the registry for each docker image layer.

Thanks for the context, at Dependabot's scale that's quite the challenge (an exciting one though!)

Is the issue only about layer calls or also due to the size of the images?(ie: pulls?)

I am asking because some images are quite large (ie: Maven) and makes me curious if we can find opportunities there as well

So we wanted to reduce layers where it made sense (ie, don't be myopically fixated on a single layer) and here I wondered if we could accomplish both RUN commands as one... so I asked Copilot to put together a PR to do that...

I think that the approach makes sense. I am not concerned about readability because EOT makes it very readable.

As part of this change, we may be able to consolidate further. See #13685

Note: the one thing I'm not sure of is whether this apt mirror that gets configured is intended to live beyond the immediate apt get call... for example, the child docker images that inherit from this may want it in order to ensure that any apt-get calls they make are also fast. If today they inherit this file, but combining would result in them not inheriting, then don't do any work. However, given that we explicitly rm apt config, I do wonder if this is getting all cleaned up anyway at the end of the layer?

When we run rm -rf /var/lib/apt/lists/*, the goal is to remove the index and metadata generated by commands such as apt-get update. This data is typically not useful for other images that depend on the base image as we usually regenerate them

However, the registry configuration created in this layer is stored in /etc/apt/sources.list.d/ubuntu.sources, which is meant to be persisted and used by downstream images. This pull request does not impact that, so it remains available for other builds and everything continues to work as expected from what I tested

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants